Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add musl target support in npm pakcage #1067

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

matteosacchetto
Copy link
Contributor

@matteosacchetto matteosacchetto commented Dec 5, 2023

Summary

In this PR i am adding support for the linux musl target in the npm package @biomejs/biome.
The idea of adding this feature was initially introduced in discussion #946, since from version 1.40 biome is now built also for the linux musl target thanks to PR #821.

In this PR i am modifying the following files:

Minore issue, though I do not know if it is solvable, on linux with npm it will install both @biomejs/cli-linux-${arch} and @biomejs/cli-linux-${arch}-musl, since npm will install based on arch and platform (which is linux for both packages).

Test Plan

I tested the isMusl() function on both linux and alpine to check it was working properly.
I checked that getName() was returning the correct names for the various platforms and arch.

I tested locally, by running npm pack and installing them locally and on alpine, the correct musl target is run.
Tested also with bun on alpine, and with pnpm.

Copy link

netlify bot commented Dec 5, 2023

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit fcfd936
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/65720491328d780008b009fe
😎 Deploy Preview https://deploy-preview-1067--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@matteosacchetto matteosacchetto marked this pull request as ready for review December 7, 2023 17:45
@ematipico
Copy link
Member

Minore issue, though I do not know if it is solvable, on linux with npm it will install both @biomejs/cli-linux-${arch} and @biomejs/cli-linux-${arch}-musl, since npm will install based on arch and platform (which is linux for both packages).

This is something we could add to our documentation, what do you think? Maybe here: https://biomejs.dev/guides/manual-installation/#supported-platforms

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's awesome @matteosacchetto, thank you so much for this contribution! 🙏

@matteosacchetto
Copy link
Contributor Author

Minore issue, though I do not know if it is solvable, on linux with npm it will install both @biomejs/cli-linux-${arch} and @biomejs/cli-linux-${arch}-musl, since npm will install based on arch and platform (which is linux for both packages).

This is something we could add to our documentation, what do you think? Maybe here: https://biomejs.dev/guides/manual-installation/#supported-platforms

I agree that it should be something to mention, though I do not know where we could mention it.
I feel like the "manual installation" section is not 100% the correct place, since it is something related to when it is installed with NPM

@ematipico
Copy link
Member

We could add a new heading ### after the installation instructions: https://biomejs.dev/guides/getting-started/#installation

@ematipico
Copy link
Member

Going to merge this, we can write the documentation in a different PR :D

@ematipico ematipico merged commit da24940 into biomejs:main Dec 7, 2023
6 checks passed
@matteosacchetto matteosacchetto deleted the npm-musl-target branch December 12, 2023 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants